CNTRLPLANE-2684: CPO etcd-upload subcommand for cloud storage upload - #8017
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
|
@jparrill: This pull request references CNTRLPLANE-2684 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.22.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
Important Review skippedAuto reviews are limited based on label configuration. 🚫 Review skipped — only excluded labels are configured. (1)
Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository YAML (base), Organization UI (inherited) Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds an Sequence Diagram(s)sequenceDiagram
participant User as "User (CLI)"
participant CPO as "control-plane-operator\n(etcd-upload)"
participant FS as "Filesystem"
participant Factory as "UploaderFactory"
participant Uploader as "Uploader\n(S3 or Azure)"
participant SDK as "Cloud SDK\n(AWS/Azure)"
participant Storage as "Cloud Storage\n(S3 or Azure Blob)"
User->>CPO: run `control-plane-operator etcd-upload --snapshot-path ... --storage-type ...`
CPO->>FS: stat/open snapshot file
FS-->>CPO: file handle
CPO->>CPO: generate object key (prefix + timestamp + ext)
CPO->>Factory: newUploader(storageType, creds, encryption)
Factory-->>CPO: Uploader instance
CPO->>Uploader: Upload(ctx, snapshotPath, key)
Uploader->>SDK: construct authenticated upload request
SDK->>Storage: perform upload
Storage-->>SDK: success (location/etag) or error
SDK-->>Uploader: upload response or error
Uploader-->>CPO: UploadResult{URL} or error
CPO-->>User: print URL or exit non-zero
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
@jparrill: This pull request references CNTRLPLANE-2684 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.22.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
Skipping CI for Draft Pull Request. |
|
@jparrill: This pull request references CNTRLPLANE-2684 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.22.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
/label tide/merge-method-squash |
4eaefc8 to
eba4817
Compare
|
/test unit |
sdminonne
left a comment
There was a problem hiding this comment.
Drop some questions and raise a couple of flags.
| input.SSEKMSKeyId = aws.String(u.kmsKeyARN) | ||
| } | ||
|
|
||
| if _, err := u.client.PutObject(ctx, input); err != nil { |
There was a problem hiding this comment.
I'm reporting here the output of our common friend: any reason for not using transfermanager.New(s3Client) as etcd-backup
There was a problem hiding this comment.
Done! Migrated to transfermanager.New(s3Client) — consistent with etcd-backup. This gives us automatic multipart upload for files >5GB, parallel part uploads, and partial retry on failure.
Note: transfermanager.PutObjectInput doesn't support IfNoneMatch (conditional writes), but since we use timestamp-based key naming ({prefix}/{unix_seconds}.db), collisions are practically impossible. Worth the trade-off for the multipart benefits.
| return nil, fmt.Errorf("--credentials-file is required for S3 storage type") | ||
| } | ||
|
|
||
| cfg, err := config.LoadDefaultConfig(context.Background(), |
There was a problem hiding this comment.
context.Background() ... Gues the NewS3Uploader should take the caller context caller... (any timeout set)?
Unsure about this. Feel free to push back but happy to hear about it.
TY!
There was a problem hiding this comment.
Good catch, done! NewS3Uploader now takes ctx context.Context as its first parameter. The caller passes the context from signal.NotifyContext so cancellation propagates properly on SIGINT/SIGTERM.
|
|
||
| cfg, err := config.LoadDefaultConfig(context.Background(), | ||
| config.WithRegion(region), | ||
| config.WithSharedCredentialsFiles([]string{credentialsFile}), |
There was a problem hiding this comment.
is credentialsFile mandatory? Can't we fallaback to role-based? I think we do elsewhere Am I wrong?
There was a problem hiding this comment.
You're right, done! credentialsFile is now optional. When empty, it falls back to the default AWS credential chain (environment variables, shared config, EC2 instance profile, IRSA, etc.) — same pattern as etcd-backup.
2c61a5e to
e82fc3a
Compare
|
Addressed reviewer comments + rebase to catch Konflux fixes |
e82fc3a to
da27c0d
Compare
|
@jparrill: This pull request references CNTRLPLANE-2684 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.22.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
/test e2e-azure-self-managed |
2 similar comments
|
/test e2e-azure-self-managed |
|
/test e2e-azure-self-managed |
|
/retest-required |
4 similar comments
|
/retest-required |
|
/retest-required |
|
/retest-required |
|
/retest-required |
|
/hold Revision 2eda006 was retested 3 times: holding |
|
/hold cancel |
|
/test e2e-azure-self-managed |
|
@bryan-cox done |
|
/retest-required |
|
/test e2e-azure-self-managed |
|
@jparrill: all tests passed! Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
|
/jira backport release-4.21 |
|
@jparrill: The following backport issues have been created: Queuing cherrypicks to the requested branches to be created after this PR merges: DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
@openshift-ci-robot: Failed to get PR patch from GitHub. This PR will need to be manually cherrypicked. Error messagestatus code 406 not one of [200], body: {"message":"Sorry, the diff exceeded the maximum number of lines (20000)","errors":[{"resource":"PullRequest","field":"diff","code":"too_large"}],"documentation_url":"https://docs.github.com/rest/pulls/pulls#get-a-pull-request","status":"406"}DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
…penshift#8017) * build(etcd-upload): add Azure Blob Storage SDK dependency Vendor the Azure Blob Storage SDK (azblob) required by the etcd-upload subcommand for Azure Blob storage support. Ref: CNTRLPLANE-2684 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Juan Manuel Parrilla Madrid <jparrill@redhat.com> * feat(etcd-upload): add CPO subcommand for cloud storage upload Add the etcd-upload subcommand to the control-plane-operator for uploading etcd snapshots to S3 and Azure Blob storage. Uses the AWS transfer manager for multipart uploads and supports SSE-KMS (S3) and encryption scopes (Azure). Credentials are optional for S3, falling back to the default AWS credential chain. Ref: CNTRLPLANE-2684 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Juan Manuel Parrilla Madrid <jparrill@redhat.com> * test(etcd-upload): add OADP integration test structure Add integration tests for the etcd-upload subcommand covering S3 and Azure Blob uploads with encryption. Includes a self-contained test runner script that creates and tears down all cloud resources automatically. Ref: CNTRLPLANE-2684 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Juan Manuel Parrilla Madrid <jparrill@redhat.com> * fix(etcd-upload): correct Agent platform resource in OADP CLI tests Fix the Agent platform resource expectation from agentclusters.infrastructure.cluster.x-k8s.io to agentclusters.capi-provider.agent-install.openshift.io. Also make the integration test runner self-contained with RBAC propagation retry logic. Ref: CNTRLPLANE-2684 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Juan Manuel Parrilla Madrid <jparrill@redhat.com> --------- Signed-off-by: Juan Manuel Parrilla Madrid <jparrill@redhat.com> Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
|
/jira backport release-4.21,release-4.20 |
|
@jparrill: Missing required branches for backport chain:
DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
Adjust code from PRs openshift#8010, openshift#8017, openshift#8040 to compile on release-4.21: - etcd-upload: migrate S3TransferAPI from transfermanager v0.1 API (PutObject/PutObjectInput) to v0.2 API (UploadObject/UploadObjectInput) resolved by go mod tidy on this branch. Regenerate mock accordingly. - etcd-backup: fix mapToTags to use url.Values for proper URL-encoding of S3 object tag keys/values (bug exposed by backported tests). - go.mod/vendor: add aws-sdk-go-v2 direct dependencies (config, s3, transfermanager) and Azure azblob SDK required by etcd-upload, then run go mod tidy + go mod vendor. Ref: CNTRLPLANE-2678 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: Juan Manuel Parrilla Madrid <jparrill@redhat.com>
Summary
etcd-uploadsubcommand to the control-plane-operator for uploading etcd snapshots to S3 and Azure Blob Storageclient-secret(JSON file) andmanaged-identity(msi-dataplane via SecretProviderClass for ARO HCP), withDefaultAzureCredentialfallbacktest/integration/oadp/withcli/andupload/subdirectoriesEpic
CNTRLPLANE-2676 - HCPEtcdBackup CRD for OADP Integration
Dependencies
Related PRs:
For the reviewers
To run the upload integration tests you need:
make control-plane-operator(builds the CPO binary)awsCLI and/orazCLIThe script creates all resources (buckets, KMS keys, SPs, storage accounts) and cleans them up on exit.
Test plan
./test/integration/oadp/run.sh upload)./test/integration/oadp/run.sh upload)🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Tests
Chores